-
Notifications
You must be signed in to change notification settings - Fork 570
Add a chapter on divergence #2067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add a chapter on divergence #2067
Conversation
3b0ec86 to
920cec4
Compare
920cec4 to
d020656
Compare
|
Thanks for the PR @jackh726; I can tell this was written carefully. It will be good to get more of this documented. In particular, it'll be good to have the fallback behavior documented. I'll leave some notes inline. Probably we'll want to move some things around. Adding more examples -- even beyond what I'll note specifically inline -- would be particularly good for this material. It's helpful when each rule has one or more concise and testable examples demonstrating exactly what the rule means to express. |
|
Thanks for the review @traviscross. Good points here, I'll work on sorting through them today/tomorrow. Happy to jump on a call at some point too, if you think any of these could use further discussion. |
a11338f to
a9d8264
Compare
|
@rustbot author |
|
Error: Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
63775d7 to
f9cf9f5
Compare
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some drive-by comments.
|
Okay, I have addressed the reviews to the best of my ability. regarding moving regarding inlining other rules (#2067 (comment)): regarding adding more examples: @rustbot review |
4040ed0 to
4f34b11
Compare
src/items/functions.md
Outdated
|
|
||
| r[items.fn.implicit-return] | ||
| If the output type is not explicitly stated, it is the [unit type]. | ||
| If the output type is not explicitly stated, it is the [unit type]. However, if the block expression is not [diverging](../divergence.md), then the output type is instead [`!`](../types/never.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In talking over this PR in the office hours, @ehuss and I puzzled over this rule. Presumably the not is misplaced, but even with that removed, we weren't clear on how to assign meaning to this given, of course:
fn f() {
loop {} // Diverging expression.
}
let _ = || -> ! { f() }; //~ ERROR mismatched types
let _: fn() -> ! = f; //~ ERROR mismatched typesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is a good catch. Indeed, the additional language is not correct for the function signature.
What I had in mind when writing this was that when type checking the function body the implicit return is ! if it is diverging. This is technically covered by my changes to expr.block.type.diverging, so I'm not sure why I thought that these changes were also needed.
I think it was motivated by this example. I guess, maybe there is an equivalent statement missing about the implicit "final type" of a block (which is by default () and otherwise ! when diverging).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be reverted, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, probably. But I wonder if there should be some additional text added somewhere that the type of the function block may not actually be the return type of the function, but rather must be coercible into the return type of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that coerce.site.return specifies that the function body is coerced to the function's return type.
I can't find the subsequent part that the function body's type must match? the function's return type (after coercion). Type checking in general isn't documented (#595), and I'm not sure which approach will make sense for that (for example, would there be a dedicated type checking chapter, or would the type checking rules be sprinkled throughout, or a mixture of both?).
I've removed this change in my local branch which I hope to push up soon. I've added this to my notes for future followup.
|
@ehuss and I talked over this PR in the lang-docs office hours this week. We're happy to be getting this documented; thanks to @jackh726 for that. To set expectations here: @ehuss is planning to make a series of editorial revisions. He'll push those onto this branch. Our feeling was that, with these editorial revisions, this is likely to merge soon after. Due to the upcoming holidays, this will happen in 2026. |
4f34b11 to
e097e7e
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This test is meant to show that matching a diverging place against the wildcard pattern does not cause a read and therefore does not cause the block to diverge. This was shown by ascribing the return type of the function to unit. But due to never-to-any coercion, that will always type check, even if the read does take place. Let's instead ascribe the function return type to never and assert that the test must fail. This way, the test would fail if the read did occur.
This tries to add consistency to if, match, break, continue, return, and loop in how the rule for divergence (and their type).
Also consistently uses periods in comments.
This helps make it clear that this is using special syntax that is normally not allowed. Also, unhide `make` so it isn't so mysterious what it is.
This particular example can be written with stable Rust, and I don't think it detracts too much from what it is trying to illustrate.
The addition here isn't quite right. See discussion at https://github.com/rust-lang/reference/pull/2067/changes#r2630177707. As a later followup, we should document that the type of the function block must be coercible into the return type of the function. (Or if there is a more generalized way to state that.)
Also clean up the links.
Rule names shouldn't be nested under other rules.
I'm not sure what happened here. The rule was originally expr.match.type.diverging.conditional, then it was requested to change to expr.match.diverging. I think that suggestion fits well here, so let's go with it.
This starts off the chapter with a direct definition of what a diverging expression is (see discussion in https://github.com/rust-lang/reference/pull/2067/changes#diff-11f5e85172f632195dd6a9a80daa852911fd3d66e788177a3bb39f6c5d7f21cfR10 about this definition). This also adds a very simple example showing divergence. Maybe not the greatest illustration, but I'd prefer to have at least a little something.
This was asked to be changed or removed in rust-lang#2067 (comment) and was removed in 5eef4bc, but I think it is useful to include the links to the appropriate rules that are relevant here. This doesn't exactly duplicate any of the rules, it is just forwarding links, and this is something we do all over the Reference. Also add a mention about panics. I'm not sure if putting this in a note is really the right approach. However, we don't directly document `panic!` yet. We are currently in discussion about adding documentation for built-in macros.
Add a rule identifier to this particular statement. Also some slight rewording for clarity.
For the most part, this seemed to be restating the expr.block.diverging rule. (At least, I couldn't determine if it was expressing something distinct.) This is an interesting property, but one that I think is already covered. So, relegate it to a note linking to the relevant text.
|
@jackh726 OK, sorry for the delay, I finally got around to doing a round of edits to clean things up and get it in the style of the Reference. Would you be able to give the changes a look over and make sure I didn't mess anything up? The commits should explain the reasoning for the changes. The only thing on my todo list for later followup is to make sure the thing we discussed about labeled breaks in #2067 (comment). There's already some rules about this, but I think there's some clarification that could be added. And I still don't 100% understand the Besides the todo at the bottom of the |
|
|
||
| r[expr.block.type] | ||
| The type of a block is the type of the final operand, or `()` if the final operand is omitted. | ||
| The type of a block is the type of its final operand; if that operand is omitted, the type is the [unit type], unless the block [diverges][expr.block.diverging], in which case it is the [never type]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't correct because of #2067 (comment)
In that example, the final operand is loop {}, which has a type of !. But, the type of the entire block is usize. I think I maybe miscommunicated when I said "I like your wording" - because it seems nice, but has this flaw.
My comment at the end is essentially the rule that needs to be here:
The type of the block ends up being the LUB of all the break (or return for fns) types, and the final expression (which depends on whether the block is diverging, whether it is () or !).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was what I was getting at when I said it could be a followup, since I don't fully understand all the bits here. The problem is that blocks with break expressions are treated separately from normal blocks in the reference, and we need to fit this all together. I tried with the following:
diff --git a/src/expressions/loop-expr.md b/src/expressions/loop-expr.md
index 3d5b9420..bfe08687 100644
--- a/src/expressions/loop-expr.md
+++ b/src/expressions/loop-expr.md
@@ -311,6 +311,9 @@ Example:
r[expr.loop.break.value]
A `break` expression is only permitted in the body of a loop, and has one of the forms `break`, `break 'label` or ([see below](#break-and-loop-values)) `break EXPR` or `break 'label EXPR`.
+r[expr.loop.break-value.implicit-value]
+In the context of a [`loop` with break expressions][expr.loop.break-value] or a [labeled block expression], a `break` without an expression is considered identical to `break` with expression `()`.
+
r[expr.loop.block-labels]
## Labeled block expressions
@@ -347,6 +350,24 @@ let result = 'block: {
};
```
+r[expr.loop.block-labels.type]
+The type of a labeled block expression is the [least upper bound] of all of the break operands, and the final operand. If the final operand is omitted, the type of the final operand defaults to the [unit type], unless the block [diverges][expr.block.diverging], in which case it is the [never type].
+
+> [!EXAMPLE]
+> ```rust
+> fn example(condition: bool) {
+> let s = String::from("owned");
+>
+> let _: &str = 'block: {
+> if condition {
+> break 'block &s; // &String coerced to &str via Deref
+> }
+> break 'block "literal"; // &'static str coerced to &str
+> };
+> }
+> ```
+
+
r[expr.loop.continue]
## `continue` expressions
@@ -394,15 +415,33 @@ let result = loop {
assert_eq!(result, 13);
```
-r[expr.loop.break-value.loop]
-In the case a `loop` has an associated `break`, it is not considered diverging, and the `loop` must have a type compatible with each `break` expression.
-`break` without an expression is considered identical to `break` with expression `()`.
+r[expr.loop.break-value.type]
+A `loop` with an associated `break` does not [diverge], and its type is the [least upper bound] of all of the break operands.
+
+> [!EXAMPLE]
+> ```rust
+> fn example(condition: bool) {
+> let s = String::from("owned");
+>
+> let _: &str = loop {
+> if condition {
+> break &s; // &String coerced to &str via Deref
+> }
+> break "literal"; // &'static str coerced to &str
+> };
+> }
+> ```
[`!`]: type.never
[`if` condition chains]: if-expr.md#chains-of-conditions
[`if` expressions]: if-expr.md
[`match` expression]: match-expr.md
[boolean type]: ../types/boolean.md
+[diverge]: divergence
[diverging]: divergence
+[labeled block expression]: expr.loop.block-labels
+[least upper bound]: coerce.least-upper-bound
+[never type]: type.never
[scrutinee]: ../glossary.md#scrutinee
[temporary values]: ../expressions.md#temporaries
+[unit type]: type.tuple.unit
diff --git a/src/type-coercions.md b/src/type-coercions.md
index 91b773ee..6bef99c3 100644
--- a/src/type-coercions.md
+++ b/src/type-coercions.md
@@ -239,6 +239,8 @@ LUB coercion is used and only used in the following situations:
+ To find the common type for a series of if branches.
+ To find the common type for a series of match arms.
+ To find the common type for array elements.
++ To find the common type for a [labeled block expression] among the break operands and the final block operand.
++ To find the common type for an [`loop` expression with break expressions] among the break operands.
+ To find the type for the return type of a closure with multiple return statements.
+ To check the type for the return type of a function with multiple return statements.
@@ -325,5 +327,7 @@ precisely.
[type cast operator]: expressions/operator-expr.md#type-cast-expressions
[`Unsize`]: std::marker::Unsize
[`CoerceUnsized`]: std::ops::CoerceUnsized
+[labeled block expression]: expr.loop.block-labels
+[`loop` expression with break expressions]: expr.loop.break-value
[method-call expressions]: expressions/method-call-expr.md
[supertraits]: items/traits.md#supertraitsDoes that look correct?
One rule I was particularly uncertain about was expr.loop.break-value.type. The statement "loop…does not diverge"" seems too strong. Does a loop with breaks that panics in all control flows, or break with diverging expressions, itself diverge? Does it require at least one control path to have a break without a diverging expression? If so, then it seems like "does not diverge" is too strongly worded. But I'm not sure how to word that. I'm wondering if it needs to be worded closer to how expr.block.diverging is done.
I'm also just guessing that the additions to the LUB list make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That diff looks good to me for the most part.
expr.loop.break-value.type is not correct though, for the reason you hinted at:
fn example() {
let _: ! = loop {
break loop {};
};
}This is the particular logic: https://github.com/rust-lang/rust/blob/ba2a7d33741a7ade4dc78e5e335d60d358cd1749/compiler/rustc_hir_typeck/src/expr.rs#L826
(may_break is only true if the break operand is not diverging)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording here can be something like
A
loopdoes not diverge if it contains anybreakexpressions with an operand that does not diverge, and its type is the [least upper bound] of all of the break operands.
|
Other than the one comment above, the changes look fine and correct to me. I don't think there is really anything related to divergence that's left out of this PR. |
It was little tricky when trying to describe diverging blocks. The compiler's implementation maintains sort of a "global state" when checking an expression and sub-expressions, which it resets on conditional things. Semantically, I think the way I worded it is much clearer than trying to match the implementation.
Happy to hear any specific feedback, Lcnr made did an initial review pass in rust-lang/project-goal-reference-expansion@4079171, so the second commit tries to address that.
Closes #271